Prevent orphaned cohort version sets on failed refreshes (#39 + backstop-ordering fix)#41
Conversation
A cohort version's member set is written to Redis with no TTL, and only receives one when a *successor* refresh completes successfully. Any refresh that dies mid-flight (Redis command timeout, pod restart, CohortTooLargeException) therefore strands the full member set in Redis forever. For multi-million-member cohorts this leaks ~0.5 GB per failed refresh, all pinned to the same cluster slot by the cohort hash tag. In one production cluster we found 58 orphaned version sets (~25 GB) for a single 9.8M-member cohort, accumulated over two months of intermittent refresh failures. Fix, in three parts: - Pending version sets self-clean: apply a 24h TTL to the new version's member set as soon as ingestion starts writing it, and PERSIST it when the version is promoted (description published). An aborted refresh now leaves nothing behind once the TTL fires. - Temp diff keys get the same backstop TTL right after SDIFFSTORE, in case the process dies before the finally-block DELs run. - Retire the previous version only after successful promotion. Previously the EXPIRE of the old (still published) version set ran in a finally block, so a refresh that failed mid-update expired the *live* version, breaking cohort reads until the next successful refresh. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The backstop TTLs were applied only after both SDIFFSTOREs completed, leaving addedKey unprotected for the full duration of the second SDIFFSTORE -- itself a multi-second blocking command on large cohorts and the likeliest point for the process to die or the command to time out. A crash there stranded a potentially full-cohort-sized, timestamp-named addedKey with no TTL, the exact leak this change set prevents. Safe reordering: SDIFFSTORE clears any TTL on its destination key, and addedKey is not a source of the second SDIFFSTORE, so arming its TTL between the two commands cannot be clobbered or affect the diff. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
kyeh-amp
left a comment
kyeh-amp
left a comment
There was a problem hiding this comment.
Approving. I traced the refresh lifecycle and the failure states all land safely: blob-write failure → no persist, no publish, pending set self-cleans, previous version untouched; crash mid-diff → temp keys and pending set self-clean; retry within 24h re-arms the TTL via the fresh writer and re-adds the same snapshot idempotently.
Verified independently:
:core:testgreen locally, including both new TTL-lifecycle tests.- Authorship preserved —
26f1232is @adrianliu0815's #39 commit unchanged. - The safe-reorder claim in
f5e7acfchecks out:SDIFFSTOREwipes its destination's TTL, andaddedKeyis neither a source nor the destination of the secondSDIFFSTORE— which is exactly the multi-second window where a crash is most likely.
Two things I particularly like: moving the previous-version retirement out of the finally closes a real failure mode (a failed refresh could expire the still-published live version), and the prev.lastModified != description.lastModified guard also quietly fixes the latent same-version case where existingCohortKey == newCohortKey and the old code would have expired the live key.
Also agree with deferring the persist-before-publish window to #40 — the publish-then-persist reorder genuinely needs the not-modified-cycle self-heal to be safe, so splitting it out is right. The residual window here is two fast O(1) commands, versus the guaranteed leak this PR fixes.
A few non-blocking suggestions inline.
| // process dies before the DELs below run, the temp keys | ||
| // self-clean instead of persisting forever. | ||
| val addedCount = redis.sdiffstore(addedKey, newCohortKey, existingCohortKey) | ||
| redis.expire(addedKey, PENDING_VERSION_TTL) |
There was a problem hiding this comment.
Non-blocking: this ordering fix is the one behavior in the PR without a pinning test — the two new tests cover the pending-TTL lifecycle, but nothing fails if these expire calls drift back below the second SDIFFSTORE. A small failure-injection test would lock it in: an InMemoryRedis subclass whose sdiffstore throws on the second call, then assert addedKey is present in expirations. Fine as a follow-up.
| redis.sadd(newCohortKey, members.toSet()) | ||
| if (!pendingTtlApplied) { | ||
| // Self-clean if this refresh dies before the version is promoted in complete() | ||
| redis.expire(newCohortKey, PENDING_VERSION_TTL) |
There was a problem hiding this comment.
Nit, non-blocking: there's still a one-batch window here — a crash between the first sadd and this expire orphans the key without a TTL. Probably not worth complicating (EXPIRE can't be armed before the key exists, and making SADD+EXPIRE atomic would mean touching the pipeline plumbing for a tiny window). Just noting it's a known residual rather than an oversight.
| // Retire the previous version only after successful promotion. A failed | ||
| // refresh must leave the previous (still published) version untouched — | ||
| // expiring it in a finally block would break reads of the live cohort. | ||
| if (finalSize > 0 && prev != null && prev.lastModified != description.lastModified) { |
There was a problem hiding this comment.
Nit, non-blocking: != is effectively > in practice, since streamCohort short-circuits lm <= existingLastModified before ever calling complete(). But > would be strictly safer against a misbehaving server handing back an older lastModified — with !=, that pathological case would expire the newer, still-live version's keys. Cheap to tighten while you're here.
Content-neutral merge: this branch already contained #39's changes (cherry-picked) and the backstop-ordering fix, so every conflict resolved to the branch side; the tree is identical to the pre-merge commit. Verified: full test suite and ktlint green, zero content diff vs 7632c55. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
4 participants
Summary
Lands @adrianliu0815's #39 with the one ordering fix we requested there, applied on our side at her request.
PERSISTat promotion, so refreshes that die mid-flight self-clean instead of stranding multi-million-member sets in Redis forever; the previous version is retired only after successful promotion instead of in afinally, so a failed refresh can no longer expire the still-published live version.f5e7acf): the temp diff keys' backstop TTLs are armed immediately after each SDIFFSTORE rather than after both —addedKeypreviously had no TTL for the entire duration of the second SDIFFSTORE, a multi-second blocking command on large cohorts and the likeliest crash point. Safe reorder: SDIFFSTORE clears its destination's TTL, andaddedKeyis not a source of the second SDIFFSTORE.Merge note
Use a merge commit, not squash — #39's head commit must remain reachable so GitHub marks #39 as merged with Adrian as author.
Testing
./gradlew checkgreen (CohortStorageTest 7/7 including #39's new TTL-lifecycle tests). The Cursor bugbot persist-ordering comment on #39 is intentionally not addressed here — the complete fix (publish-then-persist plus a not-modified-cycle self-heal) is in #40, which builds on this.🤖 Generated with Claude Code
Note
Medium Risk
Changes cohort refresh lifecycle and Redis key expiration ordering for live reads; behavior is well-tested but touches core storage paths for large cohorts.
Overview
Prevents orphaned multi-million-member cohort sets in Redis when a refresh crashes mid-flight, and stops failed refreshes from expiring the still-published live version.
In-flight cohort member sets get a 24h backstop TTL on first
SADDduring ingestion; promotion callsPERSISTso the current version never expires. Diff temp keys (added_*/removed_*) get the same TTL immediately after eachSDIFFSTORE, not after both, soaddedKeyis not left unprotected during the second blocking diff on large cohorts.Retiring the previous version (member set + blob
EXPIRE) moves out of the difffinallyblock and runs only after successful blob write, description publish, andPERSISTon the new set—so a failedcomplete()leaves v1 readable.Adds
Redis.persistto the interface and both Lettuce implementations, plusInMemoryRedissupport and twoCohortStorageTestcases for pending TTL and aborted refresh behavior.Reviewed by Cursor Bugbot for commit f5e7acf. Bugbot is set up for automated code reviews on this repo. Configure here.